review adjustments

Author: escattone

kitsune/sumo/email_utils.py

@@ -6,7 +6,7 @@
 from django.core import mail
 from django.core.exceptions import ValidationError
 from django.core.mail import EmailMultiAlternatives
-from django.core.validators import EmailValidator
+from django.core.validators import validate_email
 from django.template.loader import render_to_string
 from django.test.client import RequestFactory
 from django.utils import translation
@@ -15,15 +15,28 @@
 
 
 log = logging.getLogger("k.email")
-email_validator = EmailValidator()
 
 
-def is_valid_email(email):
+def normalize_gmail(email: str) -> str:
+    """
+    Return the given email with periods removed from its "local part"
+    if it's a gmail address, otherwise return it unchanged.
+    """
+    if email.lower().endswith("@gmail.com"):
+        # Periods are ignored in the "local part" of gmail addresses.
+        # We need to remove them before using Django's "validate_email",
+        # otherwise it might incorrectly raise a ValidationError.
+        local_part, domain = email.rsplit("@", maxsplit=1)
+        return f"{local_part.replace('.', '')}@{domain}"
+    return email
+
+
+def is_valid_email(email: str) -> bool:
     """
     Returns True if the given email address is valid, False otherwise.
     """
     try:
-        email_validator(email)
+        validate_email(normalize_gmail(email))
     except ValidationError:
         return False
     return True
@@ -34,10 +47,18 @@ def send_messages(messages):
     if not messages:
         return
 
+    # Only send each message to its valid recipients,
+    # excluding messages without any valid recipients.
+    cleaned_messages = []
+    for message in messages:
+        # Remove invalid emails and normalize gmails.
+        cleaned_to = [normalize_gmail(email) for email in message.to if is_valid_email(email)]
+        if cleaned_to:
+            message.to = cleaned_to
+            cleaned_messages.append(message)
+
     with mail.get_connection(fail_silently=True) as conn:
-        conn.send_messages(
-            list(msg for msg in messages if all(is_valid_email(email) for email in msg.to))
-        )
+        conn.send_messages(cleaned_messages)
 
 
 def safe_translation(f):

kitsune/sumo/tests/test_email_utils.py

@@ -138,25 +138,21 @@ def test_styles_inlining(self):
 class SendMessagesTests(TestCase):
 
     @patch("kitsune.sumo.email_utils.mail")
-    def test_when_messages_have_valid_email(self, mock_mail):
+    def test_send_messages(self, mock_mail):
         from_email = "notifications@support.mozilla.org"
         messages = [
-            EmailMultiAlternatives("Test", "Testing", from_email, ["ringo@beatles.org"]),
-            EmailMultiAlternatives("Test", "Testing", from_email, ["george@beatles.org"]),
-            EmailMultiAlternatives("Test", "Testing", from_email, ["paul@beatles.org"]),
+            EmailMultiAlternatives("Test", "Testing", from_email, ["beatles"]),
+            EmailMultiAlternatives(
+                "Test", "Testing", from_email, ["george.harrison.@gmail.com", "ringo"]
+            ),
+            EmailMultiAlternatives("Test", "Testing", from_email, ["paul.mccartney.@gmail.com"]),
+            EmailMultiAlternatives(
+                "Test", "Testing", from_email, ["ringo@beatles.com", "george@beatles.com"]
+            ),
         ]
         send_messages(messages)
         send_messages_mock = mock_mail.get_connection().__enter__().send_messages
-        send_messages_mock.assert_called_once_with(messages)
-
-    @patch("kitsune.sumo.email_utils.mail")
-    def test_when_message_has_invalid_email(self, mock_mail):
-        from_email = "notifications@support.mozilla.org"
-        messages = [
-            EmailMultiAlternatives("Test", "Testing", from_email, ["ringo@beatles.org"]),
-            EmailMultiAlternatives("Test", "Testing", from_email, ["george.@beatles.org"]),
-            EmailMultiAlternatives("Test", "Testing", from_email, ["paul@beatles.org"]),
-        ]
-        send_messages(messages)
-        send_messages_mock = mock_mail.get_connection().__enter__().send_messages
-        send_messages_mock.assert_called_once_with([messages[0], messages[2]])
+        send_messages_mock.assert_called_once_with([messages[1], messages[2], messages[3]])
+        self.assertEqual(messages[1].to, ["georgeharrison@gmail.com"])
+        self.assertEqual(messages[2].to, ["paulmccartney@gmail.com"])
+        self.assertEqual(messages[3].to, ["ringo@beatles.com", "george@beatles.com"])